NO-JIRA: certregenerationcontroller: Improve goroutine mgmt#1981
NO-JIRA: certregenerationcontroller: Improve goroutine mgmt#1981tchap wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tchap: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces a bespoke single-item workqueue CA bundle controller with a library-go Changes
Sequence Diagram(s)sequenceDiagram
participant Informer as Informer(s)
participant Factory as factory.Controller
participant Sync as caBundleController.sync
participant KubeAPI as Kubernetes API
Informer->>Factory: inform event (ConfigMap change)
Factory->>Sync: invoke sync(ctx, factory.SyncContext)
Sync->>KubeAPI: read/update ConfigMaps/Secrets
KubeAPI-->>Sync: response
Sync-->>Factory: sync complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cmd/certregenerationcontroller/cmd.go (1)
127-127: Minor: DuplicateconfigInformers.Start()call.
configInformers.Start(ctx.Done())is called at line 127 and again at line 168. WhileSharedInformerFactory.Start()is idempotent (safe to call multiple times), the first call could be removed since line 168 starts all informers after resources are set up.- configInformers.Start(ctx.Done()) - wg.Add(1) go func() { defer wg.Done() featureGateAccessor.Run(ctx) }()Also applies to: 168-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/cmd/certregenerationcontroller/cabundlesyncer.go(4 hunks)pkg/cmd/certregenerationcontroller/cmd.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cmd/certregenerationcontroller/cabundlesyncer.gopkg/cmd/certregenerationcontroller/cmd.go
🔇 Additional comments (4)
pkg/cmd/certregenerationcontroller/cabundlesyncer.go (2)
40-40: LGTM! Correct migration to typed queue.The refactor from the deprecated
RateLimitingInterfacetoTypedRateLimitingInterface[string]is properly implemented with the corresponding constructor and config changes.Also applies to: 52-57
82-105: LGTM! Well-structured goroutine lifecycle management.The shutdown sequence is correct:
c.queue.ShutDown()signals the worker to exit (causesGet()to returnquit=true)wg.Wait()ensures the worker completes before returningThe
HandleCrashWithContext(ctx)is a good improvement for context-aware crash handling.pkg/cmd/certregenerationcontroller/cmd.go (2)
121-133: LGTM! Correct defer ordering for graceful shutdown.The pattern is correct:
defer cancel()afterdefer wg.Wait()ensures cancel executes first (LIFO), signaling goroutines to stop before waiting. The comment at line 123 accurately documents this requirement.
170-180: LGTM! Proper WaitGroup tracking for controller goroutines.Both controller goroutines are correctly managed with
wg.Add(1)before launch anddefer wg.Done()inside, ensuring graceful shutdown coordination.
tchap
force-pushed
the
certregeneration-refactoring
branch
from
d93c607 to
c619281
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
| @@ -74,23 +80,24 @@ func NewCABundleController( | |||
| } | |||
|
|
|||
| func (c *CABundleController) Run(ctx context.Context) { | |||
There was a problem hiding this comment.
could this be rewritten to use standard factory.New() controller ?
There was a problem hiding this comment.
I guess it could, I will take a look when I have more spare time.
There was a problem hiding this comment.
Rewritten to use factory.New.
| func (c *CABundleController) Run(ctx context.Context) { | ||
| defer utilruntime.HandleCrash() | ||
|
|
||
| // FIXME: These are missing a wait group to track goroutines and handle graceful termination |
There was a problem hiding this comment.
what does graceful termination mean in this context ?
There was a problem hiding this comment.
Pretty much nothing, I don't think the command is handling signals really.
Always nice to handle signals, but not really important here, I guess. It's a bit orthogonal to handling goroutines. It's always good to have all goroutines terminated when Run returns.
tchap
force-pushed
the
certregeneration-refactoring
branch
2 times, most recently
from
d9eb289 to
c47d6d6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cmd/certregenerationcontroller/cmd.go (1)
127-127: DuplicateconfigInformers.Start()calls.
configInformers.Start(ctx.Done())is called at line 127 and again at line 167. WhileSharedInformerFactory.Start()is idempotent (it safely no-ops for already-started informers), having it called twice may confuse future readers about the intended lifecycle.Consider removing the first call at line 127 if the feature gate informers can wait until line 167, or add a brief inline comment explaining why the early start is required for
featureGateAccessorto initialize.Also applies to: 167-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/certregenerationcontroller/cmd.go` at line 127, Duplicate call to configInformers.Start(ctx.Done()) (SharedInformerFactory.Start) appears at the earlier location and again later near where featureGateAccessor is used; remove the first call so the factory is only started once at the intended lifecycle point (the later start near featureGateAccessor) OR, if the early start is actually required to initialize featureGateAccessor, keep the early configInformers.Start call and add a brief inline comment next to that call referencing featureGateAccessor initialization (e.g., "start early to allow featureGateAccessor to initialize") so future readers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/certregenerationcontroller/cmd.go`:
- Line 127: Duplicate call to configInformers.Start(ctx.Done())
(SharedInformerFactory.Start) appears at the earlier location and again later
near where featureGateAccessor is used; remove the first call so the factory is
only started once at the intended lifecycle point (the later start near
featureGateAccessor) OR, if the early start is actually required to initialize
featureGateAccessor, keep the early configInformers.Start call and add a brief
inline comment next to that call referencing featureGateAccessor initialization
(e.g., "start early to allow featureGateAccessor to initialize") so future
readers understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4b1f0a0-2060-497a-b330-d1e514dcd7aa
📒 Files selected for processing (2)
pkg/cmd/certregenerationcontroller/cabundlesyncer.gopkg/cmd/certregenerationcontroller/cmd.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cmd/certregenerationcontroller/cmd.go (1)
127-127: DuplicateconfigInformers.Start()call is safe but redundant.
configInformers.Start(ctx.Done())is called at both line 127 and line 167. WhileSharedInformerFactory.Start()is idempotent (subsequent calls are no-ops for already-started informers), the duplicate call adds confusion. Consider removing line 167 since line 127 already starts the config informers before the feature gate accessor needs them.🔧 Suggested fix
// We can't start informers until after the resources have been requested. Now is the time. kubeAPIServerInformersForNamespaces.Start(ctx.Done()) dynamicInformers.Start(ctx.Done()) - configInformers.Start(ctx.Done())Also applies to: 167-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/certregenerationcontroller/cmd.go` at line 127, The call to configInformers.Start(ctx.Done()) is duplicated; remove the second invocation (the later call near where feature gate accessor is initialized) so only the initial configInformers.Start(ctx.Done()) at the earlier setup remains; update any nearby comments if needed to reflect a single start and ensure no other code relies on the redundant call in functions/methods around cmd.go where configInformers.Start is referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/certregenerationcontroller/cmd.go`:
- Line 127: The call to configInformers.Start(ctx.Done()) is duplicated; remove
the second invocation (the later call near where feature gate accessor is
initialized) so only the initial configInformers.Start(ctx.Done()) at the
earlier setup remains; update any nearby comments if needed to reflect a single
start and ensure no other code relies on the redundant call in functions/methods
around cmd.go where configInformers.Start is referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 14c47469-79f7-4a02-9e99-9315e7cd88ce
📒 Files selected for processing (2)
pkg/cmd/certregenerationcontroller/cabundlesyncer.gopkg/cmd/certregenerationcontroller/cmd.go
tchap
force-pushed
the
certregeneration-refactoring
branch
2 times, most recently
from
9824745 to
d08e211
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/certregenerationcontroller/cmd.go`:
- Around line 121-130: The startup currently blocks waiting for feature gates
without honoring context cancellation, so modify the bootstrap that waits for
InitialFeatureGatesObserved to also select on ctx.Done() (the cancellable
context created by ctx, cancel) so shutdown unblocks immediately; specifically,
update the logic around featureGateAccessor.Run(ctx) and the subsequent wait for
InitialFeatureGatesObserved() to use a select that listens for either
featureGateAccessor.InitialFeatureGatesObserved() completion (or its returned
channel/boolean) or ctx.Done(), and return/cleanup when ctx is cancelled instead
of waiting the full timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: db861c3c-cc07-4515-8bc7-804b3f1ae177
📒 Files selected for processing (2)
pkg/cmd/certregenerationcontroller/cabundlesyncer.gopkg/cmd/certregenerationcontroller/cmd.go
Use a WaitGroup to manage all goroutines.
Rewrite the controller init to use factory.New to remove unnecessary boilerplate code.
tchap
force-pushed
the
certregeneration-refactoring
branch
from
d08e211 to
e6bd2b1
Compare
|
/retest |
|
@tchap: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Also use typed queues, the untyped are deprecated.
This is just refactoring and handling some todos.
I took the same approach as implemented in core k8s controllers.
Summary by CodeRabbit